Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix regression in overload resolution picking eta-expanded method #21847

Closed
wants to merge 1 commit into from

Conversation

dwijnand
Copy link
Member

Fixes #21727

@dwijnand dwijnand requested a review from odersky October 26, 2024 13:28
@dwijnand dwijnand marked this pull request as ready for review October 26, 2024 13:29
@dwijnand
Copy link
Member Author

I didn't look at what change or changes caused the code to regress, seeing as it was so many versions ago. So I just took this as it stood. We seem to have a compare algorithm, that doesn't take an expected pt type into consideration. And then we seem to discard an eta-expandable method over a nilary contextual method, optimistically expecting the required implicts to be (unambiguously) present. So I thought I attempted some amount of eta-expansion consideration, but I'm open to pointers and any other suggested changes.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the reasoning behind the change, and the code is fine. But this would be a rather sweeping spec change in a very sensitive area. We risk

  • code changing behavior at runtime because another alternative is now chosen
  • compiletime blowouts since converting to function types when comparing alternatives can be expensive and we know that alternative comparison can already be very bad.

So my gut feeling is not to go ahead with this. It's too risky. When it comes to overloading resolution we should give high priority to the status quo and only change something when we have bad regressions, like existing programs changing runtime behavior.

I left a comment on the original issue why this happens. The problem is, the program in #21727 originally compiled only because of a very dubious addition of unknown origins that has since been removed. Since it seems OpenCI was OK with this removal, I would say let's treat that as the status quo to preserve.

@dwijnand
Copy link
Member Author

I would say let's treat that as the status quo to preserve.

ok

@dwijnand dwijnand closed this Oct 28, 2024
@dwijnand dwijnand deleted the regr-overload-eta branch October 28, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in eta-expansion while overloading as of 3.3.4
2 participants